QUCOL Onboarding #993

qucol-odoo · 2025-10-20T11:40:11Z

No description provided.

robodoo · 2025-10-20T11:40:13Z

alan-odoo

Here is a review of your code. You went through the tutorials without much difficulty so I've been a little bit more touchy on your PR 😃.

Your branch is very clean! And we're not going to dirty it. Then can you do the corrections in the right commits ? In the end, your branch shouldn't contain new commit. If you meet some issue doing it, ping me 😃.

Thanks for your job and for helping your teammates!

alan-odoo · 2025-10-22T13:19:02Z

estate/models/__init__.py

+from . import estate_properties
+from . import estate_property_types
+from . import estate_property_tags
+from . import estate_property_offers


They should be listed alphabetically.

alan-odoo · 2025-10-22T14:02:15Z

estate/models/estate_property.py

+DEFAULT_GARDEN_AREA = 10
+DEFAULT_GARDEN_ORIENTATION = "north"
+


That's the right way to write constants but I am not sure to understand why it is required. They are used only in _update_garden_area_and_orientation, can they be defined directly in that method?

alan-odoo · 2025-10-22T14:03:18Z

estate/models/estate_property.py

+        selection=[('north', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')]
+    )
+    total_living_area = fields.Integer(compute="_compute_total_area")
+


No need of that blank line 😃.

alan-odoo · 2025-10-22T14:13:25Z

estate/models/estate_properties.py

+            record.total_living_area = record.living_area + record.garden_area
+
+    @api.depends("offer_ids")
+    def _get_highest_price(self):


As this method is used to compute the best_price, it should be named _compute_best_offer 😃, that way we know easily 1. it computes field and 2 which field.

FYI, _get or get methods should return an object and should not modify the database. For example, _get_res_user should only return a res_user and do nothing else.

alan-odoo · 2025-10-22T14:28:00Z

estate/models/estate_property.py

+            record.best_offer = max(record.offer_ids.mapped("price")) if record.offer_ids else 0
+
+    @api.onchange("garden")
+    def _update_garden_area_and_orientation(self):


alan-odoo · 2025-10-24T10:57:32Z

estate/views/estate_property_type_views.xml

+        <field name="arch" type="xml">
+            <form string="Property type">
+                <sheet>
+                    <button name="%(estate.estate_property_offer_action)d" string="Offers" type="action" title="Offers list" invisible="not offer_count > 0"/>


Suggested change

<button name="%(estate.estate_property_offer_action)d" string="Offers" type="action" title="Offers list" invisible="not offer_count > 0"/>

<button name="%(estate.estate_property_offer_action)d" string="Offers" type="action" title="Offers list" invisible="offer_count == 0"/>

alan-odoo · 2025-10-24T11:05:58Z

estate/views/estate_property_type_views.xml

+                        <field name="name"/>
+                    </h1>
+                    <group>
+                        <field name="offer_count"/>


Maybe you can test to add it inside your oe_stat_button ? (not a priority). You can find a solution here (line 847)

alan-odoo · 2025-10-24T11:12:50Z

estate/views/estate_res_user_views.xml

@@ -0,0 +1,21 @@
+<odoo>
+    <record id="estate_res_user_view_form" model="ir.ui.view">


You can call it as its parent. Other modules will refer to this one as estate.res_user_view_form. So here we know that the main form of the user has been modified and this is not a new one.

Suggested change

<record id="estate_res_user_view_form" model="ir.ui.view">

<record id="res_user_view_form" model="ir.ui.view">

alan-odoo · 2025-10-24T11:15:19Z

estate/__manifest__.py

+    ],
+    'data': [
+        'security/ir.model.access.csv',
+


No need of this blank line. Do not forget that a lot of developers are working on the project and we must avoid unnecessary changes.

alan-odoo · 2025-10-24T12:44:31Z

estate_account/models/estate_property.py

+        taken_ids = set()
+        for invoice in self.env["account.move"].search([]):
+            if (match := re.match(r"Invoice\s+(\d+)", invoice.name)):
+                taken_ids.add(int(match.group(1)))
+        i = 1
+        while i in taken_ids:
+            i += 1
+        return f"Invoice {i}"


Do you want to select the first number missing in the sequence or the one above the maximum ?
If it is the last case, I would do something like that to avoid too many loops.

Suggested change

taken_ids = set()

for invoice in self.env["account.move"].search([]):

if (match := re.match(r"Invoice\s+(\d+)", invoice.name)):

taken_ids.add(int(match.group(1)))

i = 1

while i in taken_ids:

i += 1

return f"Invoice {i}"

invoice = self.env["account.move"].search([("name", "like", "Invoice %")], order='id desc', limit=1)

if invoice and (match := re.match(r"Invoice (\d+)", invoice.name)):

return f"Invoice {match.group(1)}"

return "Invoice 1"

alan-odoo

Hi @qucol-odoo 😃,

Just a quick review. Those changes are not a priority as we are arriving at the end of the onboarding. It's just to let you know about other good practices.

About the title of your PR, its pattern is the same as the one of the commits. [IMP/FIX/ADD/...]: applications: what the PR is doing (starting with a verb). Here for the applications you could write estate{_account}. We use {} to avoid estate, estate_account. We squash them. And if there are to much app, you just write * in the title and in the description you write * = all the names of the apps.
And for the description you should summarize the content of your commits. The description must mainly explain why you are doing your modifications and not how you reached the goal.
Once again no need to change it now, it's just for you to know.

Cheers !

alan-odoo · 2025-10-31T07:35:04Z

estate/tests/test_estate_property.py

+    def create_sold_property(cls):
+        cls.sold_property = cls.env["estate.property"].create([{
+            "name": "Sold Property",
+            "state": "new",
+            "expected_price": 4000.0
+        }])
+        cls.env["estate.property.offer"].create([{
+            "partner_id": ADMINISTRATOR_ID,
+            "property_id": cls.sold_property.id,
+            "status": "accepted",
+            "price": 4000.0
+        }])
+        cls.sold_property.state = "sold"


You should create functions/methods only if they are used several times or overridden. I must say that before Odoo I liked creating functions because according to me, it made the code more readable. But they asked me to stop 😅 because it creates a ping pong effect and you should be able to read as much code as possible at once.

alan-odoo · 2025-10-31T08:00:19Z

estate/tests/test_estate_property.py

+from odoo.tests import tagged, Form
+
+
+ADMINISTRATOR_ID = 3


Data created in xml/csv have an external_id. This is the id of the xml record. It can be easily found. For example here you can search `model="res.partner"' in all your xml files and then you can check the demo data.

When you need to cache a record by its id you can used the ref method of self.env. It contacts the ir_model_data table which contains all the external ids and with the other information of the record in that table its able to find where is the record asked.

Also we usually do not write global constants. So you could just put it inside the setUpClass.

Suggested change

ADMINISTRATOR_ID = 3

ADMINISTRATOR_ID = self.env.ref("base.partner_admin").id

alan-odoo · 2025-10-31T10:11:51Z

estate/tests/test_estate_property.py

+        for offer in correct_offers:
+            try:
+                self.env["estate.property.offer"].create([offer])
+            except UserError:
+                self.fail()


No need to catch the exception 😃, or maybe I'm missing something. If an exception occurs the test doesn't pass. As it can make a code a bit less readable, do not hesitate to write why you do that. Do not forget always why and not how as every developer should be able to read your code, but sometimes the purpose may not be clear.

alan-odoo · 2025-10-31T10:15:58Z

estate/views/estate_property_views.xml

+                <field name="postcode"/>
+                <field name="expected_price"/>
+                <field name="bedrooms"/>
+                <field name="living_area" filter_domain="['|', ('living_area', '=', self), ('living_area', '&gt;', self)]"/>


Could you do this?

Suggested change

<field name="living_area" filter_domain="['|', ('living_area', '=', self), ('living_area', '>', self)]"/>

<field name="living_area" filter_domain="[('living_area', '>=', self)]"/>

Mathilde411 changed the title ~~Chapter 2~~ QUCOL Onboarding Oct 20, 2025

Mathilde411 requested a review from alan-odoo October 20, 2025 11:44

qucol-odoo force-pushed the 19.0-onboarding-qucol branch from 46fe336 to 5cda5fe Compare October 20, 2025 14:02

qucol-odoo added 2 commits October 20, 2025 16:05

[ADD] estate: Chapter 2

326711a

[IMP] estate: Chapter 3

04fcfac

qucol-odoo force-pushed the 19.0-onboarding-qucol branch 2 times, most recently from ad8ec7f to 3a01b0f Compare October 20, 2025 14:12

[IMP] estate: Chapter 4

a14ca42

qucol-odoo force-pushed the 19.0-onboarding-qucol branch 4 times, most recently from d0a617c to 0cf2ff3 Compare October 22, 2025 14:59

qucol-odoo closed this Oct 22, 2025

qucol-odoo reopened this Oct 22, 2025

qucol-odoo force-pushed the 19.0-onboarding-qucol branch 5 times, most recently from eff89b1 to 2faaaee Compare October 24, 2025 11:06

alan-odoo reviewed Oct 24, 2025

View reviewed changes

qucol-odoo force-pushed the 19.0-onboarding-qucol branch 3 times, most recently from 327b3f5 to 2a0540b Compare October 30, 2025 09:51

qucol-odoo added 2 commits October 30, 2025 13:35

[IMP] estate: Chapter 5

8d89c74

[IMP] estate: Chapter 6

f1039c9

qucol-odoo force-pushed the 19.0-onboarding-qucol branch 2 times, most recently from 2eeb5b3 to 7784a59 Compare October 30, 2025 15:39

qucol-odoo added 2 commits October 30, 2025 16:54

[IMP] estate: Chapter 7

89e7d4d

[IMP] estate: Chapter 8

c801045

qucol-odoo added 2 commits October 30, 2025 16:54

[IMP] estate: Chapter 9

f9cf0c7

[IMP] estate: Chapter 10

96568d0

qucol-odoo force-pushed the 19.0-onboarding-qucol branch from 7784a59 to 3e9e034 Compare October 30, 2025 15:56

alan-odoo reviewed Oct 31, 2025

View reviewed changes

qucol-odoo added 8 commits October 31, 2025 14:21

[IMP] estate: Chapter 11

8edf702

[IMP] estate: Chapter 12

04e09e5

[ADD] estate.account: Chapter 13

ba956ab

[IMP] estate: Chapter 14

bdfb5af

[CLN] estate: Chapter 15

e2b32f8

[IMP] estate: Unit Tests

d226474

[IMP] estate: Set garden_area and garden_orientation in Settings

b91bfa6

[IMP] estate: Define module data

601b9a6

qucol-odoo force-pushed the 19.0-onboarding-qucol branch from 3e9e034 to 601b9a6 Compare October 31, 2025 13:23

		DEFAULT_GARDEN_AREA = 10
		DEFAULT_GARDEN_ORIENTATION = "north"

	<button name="%(estate.estate_property_offer_action)d" string="Offers" type="action" title="Offers list" invisible="not offer_count > 0"/>
	<button name="%(estate.estate_property_offer_action)d" string="Offers" type="action" title="Offers list" invisible="offer_count == 0"/>

		@@ -0,0 +1,21 @@
		<odoo>
		<record id="estate_res_user_view_form" model="ir.ui.view">

	<record id="estate_res_user_view_form" model="ir.ui.view">
	<record id="res_user_view_form" model="ir.ui.view">

	ADMINISTRATOR_ID = 3
	ADMINISTRATOR_ID = self.env.ref("base.partner_admin").id

	<field name="living_area" filter_domain="['\|', ('living_area', '=', self), ('living_area', '>', self)]"/>
	<field name="living_area" filter_domain="[('living_area', '>=', self)]"/>

QUCOL Onboarding #993

Are you sure you want to change the base?

QUCOL Onboarding #993

Uh oh!

Conversation

qucol-odoo commented Oct 20, 2025

Uh oh!

robodoo commented Oct 20, 2025

Uh oh!

alan-odoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alan-odoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alan-odoo left a comment •

edited

Loading